-
Notifications
You must be signed in to change notification settings - Fork 15
perf(profiling)!: use zstd for compression #1065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
perf(profiling)!: use zstd for compression #1065
Conversation
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2025-10-09 04:32:09 Comparing candidate commit 868f0dc in PR branch Found 13 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 2 unstable metrics. scenario:benching serializing traces from their internal representation to msgpack
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:normalization/normalize_name/normalize_name/Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Lo...
scenario:normalization/normalize_name/normalize_name/bad-name
scenario:normalization/normalize_name/normalize_name/good
scenario:normalization/normalize_service/normalize_service/test_ASCII
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
Group 14
Group 15
BaselineOmitted due to size. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1065 +/- ##
==========================================
+ Coverage 71.68% 71.84% +0.15%
==========================================
Files 355 357 +2
Lines 56312 56751 +439
==========================================
+ Hits 40369 40771 +402
- Misses 15943 15980 +37
🚀 New features to boost your workflow:
|
80c8f07
to
0d6cabc
Compare
UploadCompression::Zstd => Self { | ||
buffer: Vec::with_capacity(TEMPORARY_BUFFER_SIZE), | ||
compressor: Compressor::Zstd { | ||
// A level of 0 uses zstd's default (currently 3). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the level 3 from the upstream zstd C library? If yes, level 3 is good. I'm asking because there are different zstd implementations out there which assign different meanings to different levels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at least as far as I understood it.
datadog-profiling/src/serializer/compressed_streaming_encoder.rs
Outdated
Show resolved
Hide resolved
d301705
to
8fa167e
Compare
8788621
to
714ecc0
Compare
5d54a41
to
9cafb27
Compare
714ecc0
to
5ed02e4
Compare
9cafb27
to
c4c8301
Compare
064a512
to
6f72248
Compare
49d50fa
to
9fa834b
Compare
a13e0dd
to
b751d09
Compare
9fa834b
to
35933df
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. To override this behavior, add the keep-open label or update the PR. |
b751d09
to
958b210
Compare
6630172
to
c2a9233
Compare
c2a9233
to
868f0dc
Compare
What does this PR do?
Converts from lz4 compression to zstd for file uploads in the profiler.
Breaking Change
This is a breaking change in the sense that tooling and tests may have previously assumed that the compression is lz4. Using an lz4 decompressor on zstd input will obviously fail.
Motivation
tl;dr Generally faster while creating smaller files.
zstd may have a better tradeoff for performance to compressed size.
Here are some benchmarks on compressors for a CPU profile which I cannot share publicly yet. We're working on anonymizing it so we can commit these to the repository. Throughput is in MiB/s. Utility is the compression ratio multiplied by throughput. This is on an Apple M1 Max.
I tested zstd-5 and zstd-6 informally, and compression rations were incrementally better, but you lose throughput steadily. Sticking to something in the level 1-4 range seems better.
And here's from another, also not public set of data:
My personal opinion is that zstd-1 is a safe upgrade. The performance is roughly similar to lz4, but the compression is much better.. Moving to a higher level is something that could be discussed.
Additional Notes
Currently lacks tests, pushed to see if this will cause build issues or significant artifact size bloat.
How to test the change?
As noted in Breaking Change, this adjusts the outputs to be zstd compressed instead of lz4 compressed. Tests which work on compressed files need updated.